path: support file urls - #52672
Closed
avivkeller wants to merge 4 commits into
Closed
Conversation
Collaborator
|
Review requested:
|
nodejs-github-bot
added
needs-ci
avivkeller
added
the
semver-minor
github-actions
Bot
added
request-ci-failed
Contributor
Failed to start CI⚠ No approving reviews found ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/8823064389 |
Member
Author
|
That's a good sign (for security) [i think] Do CIs now need approving reviews? 🎉 |
Member
|
They don't strictly need it. Collaborators can still start CI jobs from Jenkins. |
aduh95
removed
the
request-ci-failed
jasnell
approved these changes
Apr 27, 2024
LiviaMedeiros
approved these changes
Apr 27, 2024
LiviaMedeiros
left a comment
LiviaMedeiros
left a comment
Member
There was a problem hiding this comment.
LGTM with or without the following nits.
anonrig
requested changes
Apr 27, 2024
anonrig
left a comment
anonrig
left a comment
Member
There was a problem hiding this comment.
I'd like to see a benchmark result before merging a PR like this that effects every single function in Node.js
Member
|
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1534/ Results |
This comment was marked as duplicate.
This comment was marked as duplicate.
Member
LiviaMedeiros
approved these changes
Apr 28, 2024
LiviaMedeiros
left a comment
LiviaMedeiros
left a comment
Member
There was a problem hiding this comment.
Benchmark CI results
confidence improvement accuracy (*) (**) (***)
path/basename-posix.js n=100000 pathext='' -0.55 % ±1.29% ±1.71% ±2.20%
path/basename-posix.js n=100000 pathext='/' *** -1.38 % ±0.78% ±1.03% ±1.32%
path/basename-posix.js n=100000 pathext='/foo' * -1.16 % ±1.07% ±1.41% ±1.82%
path/basename-posix.js n=100000 pathext='/foo/.bar.baz' 0.19 % ±1.27% ±1.68% ±2.16%
path/basename-posix.js n=100000 pathext='/foo/.bar.baz|.baz' -0.87 % ±1.35% ±1.79% ±2.30%
path/basename-posix.js n=100000 pathext='/foo/bar/baz/asdf/quux.html' -0.10 % ±1.34% ±1.77% ±2.28%
path/basename-posix.js n=100000 pathext='/foo/bar/baz/asdf/quux.html|.html' ** -2.45 % ±1.68% ±2.22% ±2.85%
path/basename-posix.js n=100000 pathext='foo' *** -1.88 % ±0.75% ±0.99% ±1.27%
path/basename-posix.js n=100000 pathext='foo/bar.' -0.59 % ±0.81% ±1.07% ±1.38%
path/basename-posix.js n=100000 pathext='foo/bar.|.' ** -0.98 % ±0.72% ±0.94% ±1.22%
path/basename-win32.js n=100000 pathext='' ** -1.11 % ±0.82% ±1.08% ±1.39%
path/basename-win32.js n=100000 pathext='\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux.html' -0.05 % ±0.93% ±1.23% ±1.58%
path/basename-win32.js n=100000 pathext='\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux.html|.html' -0.75 % ±1.16% ±1.52% ±1.96%
path/basename-win32.js n=100000 pathext='C:\\\\' -0.10 % ±0.81% ±1.06% ±1.37%
path/basename-win32.js n=100000 pathext='C:\\\\foo' *** -2.59 % ±1.06% ±1.40% ±1.80%
path/basename-win32.js n=100000 pathext='D:\\\\foo\\\\.bar.baz' * -1.10 % ±0.85% ±1.12% ±1.44%
path/basename-win32.js n=100000 pathext='E:\\\\foo\\\\.bar.baz|.baz' ** -1.50 % ±0.90% ±1.19% ±1.53%
path/basename-win32.js n=100000 pathext='foo' *** -0.62 % ±0.35% ±0.46% ±0.59%
path/basename-win32.js n=100000 pathext='foo\\\\bar.' * -0.86 % ±0.75% ±0.99% ±1.28%
path/basename-win32.js n=100000 pathext='foo\\\\bar.|.' ** -1.45 % ±1.02% ±1.35% ±1.73%
path/dirname-posix.js n=100000 path='' ** -1.35 % ±0.80% ±1.06% ±1.36%
path/dirname-posix.js n=100000 path='/' *** -1.48 % ±0.73% ±0.97% ±1.24%
path/dirname-posix.js n=100000 path='/foo' *** -1.54 % ±0.56% ±0.73% ±0.94%
path/dirname-posix.js n=100000 path='/foo/bar' *** -1.23 % ±0.66% ±0.87% ±1.12%
path/dirname-posix.js n=100000 path='/foo/bar/baz/asdf/quux' * -1.59 % ±1.29% ±1.70% ±2.18%
path/dirname-posix.js n=100000 path='foo' *** -1.81 % ±0.52% ±0.69% ±0.89%
path/dirname-posix.js n=100000 path='foo/bar' *** -1.43 % ±0.35% ±0.46% ±0.60%
path/dirname-win32.js n=100000 path='' *** -4.07 % ±1.21% ±1.59% ±2.04%
path/dirname-win32.js n=100000 path='\\\\' *** -1.45 % ±0.81% ±1.07% ±1.38%
path/dirname-win32.js n=100000 path='\\\\foo' ** -1.22 % ±0.86% ±1.13% ±1.45%
path/dirname-win32.js n=100000 path='C:\\\\foo\\\\bar' -0.82 % ±0.90% ±1.18% ±1.52%
path/dirname-win32.js n=100000 path='D:\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux' -0.65 % ±1.09% ±1.44% ±1.85%
path/dirname-win32.js n=100000 path='foo' *** -1.90 % ±1.05% ±1.39% ±1.78%
path/dirname-win32.js n=100000 path='foo\\\\bar' -0.42 % ±0.59% ±0.78% ±1.01%
path/extname-posix.js n=100000 path='' *** -4.55 % ±2.46% ±3.25% ±4.18%
path/extname-posix.js n=100000 path='/' *** -2.05 % ±1.05% ±1.38% ±1.78%
path/extname-posix.js n=100000 path='/foo' *** -4.64 % ±0.87% ±1.15% ±1.48%
path/extname-posix.js n=100000 path='/foo/bar/baz/asdf/quux.foobarbazasdfquux' -1.14 % ±1.64% ±2.16% ±2.77%
path/extname-posix.js n=100000 path='/foo/bar/baz/asdf/quux' 0.24 % ±1.28% ±1.69% ±2.17%
path/extname-posix.js n=100000 path='foo/.bar.baz' 0.77 % ±1.10% ±1.45% ±1.86%
path/extname-posix.js n=100000 path='foo/bar/...baz.quux' -1.58 % ±1.62% ±2.14% ±2.75%
path/extname-posix.js n=100000 path='foo/bar/..baz.quux' * -1.75 % ±1.72% ±2.27% ±2.92%
path/extname-posix.js n=100000 path='index.html' -0.62 % ±1.28% ±1.68% ±2.16%
path/extname-posix.js n=100000 path='index' 0.13 % ±1.16% ±1.53% ±1.96%
path/extname-win32.js n=100000 path='' *** -1.99 % ±1.03% ±1.36% ±1.75%
path/extname-win32.js n=100000 path='\\\\' *** -3.51 % ±1.45% ±1.91% ±2.46%
path/extname-win32.js n=100000 path='\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux.foobarbazasdfquux' -0.54 % ±1.47% ±1.94% ±2.49%
path/extname-win32.js n=100000 path='C:\\\\foo' * -0.99 % ±0.83% ±1.10% ±1.41%
path/extname-win32.js n=100000 path='D:\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux' 0.36 % ±1.02% ±1.34% ±1.72%
path/extname-win32.js n=100000 path='foo\\\\.bar.baz' 0.18 % ±1.17% ±1.54% ±1.98%
path/extname-win32.js n=100000 path='foo\\\\bar\\\\...baz.quux' 0.18 % ±1.07% ±1.41% ±1.82%
path/extname-win32.js n=100000 path='foo\\\\bar\\\\..baz.quux' * -1.29 % ±1.16% ±1.53% ±1.96%
path/extname-win32.js n=100000 path='index.html' * -1.05 % ±0.84% ±1.11% ±1.42%
path/extname-win32.js n=100000 path='index' *** -2.03 % ±1.05% ±1.38% ±1.77%
path/format-posix.js n=1000000 props='/|/home/user/dir|index.html|.html|index' 0.18 % ±0.46% ±0.61% ±0.78%
path/format-win32.js n=1000000 props='C:\\\\|C:\\\\path\\\\dir|index.html|.html|index' 0.15 % ±0.39% ±0.51% ±0.65%
path/isAbsolute-posix.js n=100000 path='.' *** -2.05 % ±0.85% ±1.12% ±1.44%
path/isAbsolute-posix.js n=100000 path='' *** -3.72 % ±1.05% ±1.38% ±1.78%
path/isAbsolute-posix.js n=100000 path='/baz/..' *** -1.40 % ±0.69% ±0.91% ±1.17%
path/isAbsolute-posix.js n=100000 path='/foo/bar' *** -2.54 % ±0.82% ±1.08% ±1.39%
path/isAbsolute-posix.js n=100000 path='bar/baz' *** -2.25 % ±0.71% ±0.93% ±1.20%
path/isAbsolute-win32.js n=100000 path='.' *** -1.45 % ±0.64% ±0.84% ±1.08%
path/isAbsolute-win32.js n=100000 path='' -1.14 % ±1.15% ±1.52% ±1.95%
path/isAbsolute-win32.js n=100000 path='//server' *** -6.37 % ±1.03% ±1.36% ±1.75%
path/isAbsolute-win32.js n=100000 path='bar\\\\baz' *** -1.83 % ±0.46% ±0.61% ±0.78%
path/isAbsolute-win32.js n=100000 path='C:\\\\baz\\\\..' * -0.54 % ±0.43% ±0.57% ±0.73%
path/isAbsolute-win32.js n=100000 path='C:baz\\\\..' *** -0.90 % ±0.44% ±0.58% ±0.75%
path/join-posix.js n=100000 paths='/foo|bar||baz/asdf|quux|..' 0.29 % ±0.56% ±0.74% ±0.95%
path/join-win32.js n=100000 paths='C:\\\\foo|bar||baz\\\\asdf|quux|..' *** -1.12 % ±0.48% ±0.64% ±0.82%
path/makeLong-win32.js n=100000 path='\\\\\\\\?\\\\foo' -0.40 % ±0.54% ±0.72% ±0.92%
path/makeLong-win32.js n=100000 path='\\\\\\\\foo\\\\bar' -0.49 % ±0.77% ±1.01% ±1.30%
path/makeLong-win32.js n=100000 path='C:\\\\foo' -0.49 % ±1.46% ±1.93% ±2.48%
path/makeLong-win32.js n=100000 path='foo\\\\bar' -0.30 % ±0.42% ±0.55% ±0.70%
path/normalize-posix.js n=100000 path='.' *** -1.70 % ±0.61% ±0.81% ±1.04%
path/normalize-posix.js n=100000 path='' *** -1.73 % ±0.92% ±1.22% ±1.57%
path/normalize-posix.js n=100000 path='/../' *** -1.27 % ±0.45% ±0.60% ±0.77%
path/normalize-posix.js n=100000 path='/foo' ** -1.62 % ±1.03% ±1.36% ±1.75%
path/normalize-posix.js n=100000 path='/foo/bar' *** -2.05 % ±0.83% ±1.10% ±1.42%
path/normalize-posix.js n=100000 path='/foo/bar//baz/asdf/quux/..' *** 1.49 % ±0.67% ±0.88% ±1.13%
path/normalize-win32.js n=100000 path='.' -0.28 % ±1.13% ±1.49% ±1.91%
path/normalize-win32.js n=100000 path='' * -1.65 % ±1.29% ±1.71% ±2.19%
path/normalize-win32.js n=100000 path='C:\\\\..\\\\' * -1.15 % ±0.99% ±1.31% ±1.68%
path/normalize-win32.js n=100000 path='C:\\\\foo' *** -1.48 % ±0.87% ±1.15% ±1.48%
path/normalize-win32.js n=100000 path='C:\\\\foo\\\\bar' *** -1.85 % ±0.77% ±1.02% ±1.31%
path/normalize-win32.js n=100000 path='C:\\\\foo\\\\bar\\\\\\\\baz\\\\asdf\\\\quux\\\\..' 0.02 % ±0.54% ±0.72% ±0.92%
path/parse-posix.js n=100000 path='' *** -3.53 % ±1.20% ±1.59% ±2.04%
path/parse-posix.js n=100000 path='/' *** -1.68 % ±0.70% ±0.92% ±1.18%
path/parse-posix.js n=100000 path='/foo' * -0.73 % ±0.57% ±0.75% ±0.97%
path/parse-posix.js n=100000 path='/foo/bar.baz' -0.59 % ±1.24% ±1.63% ±2.10%
path/parse-posix.js n=100000 path='/foo/bar/baz/asdf/.quux' 1.10 % ±1.57% ±2.07% ±2.66%
path/parse-posix.js n=100000 path='foo/.bar.baz' * -1.90 % ±1.66% ±2.19% ±2.82%
path/parse-posix.js n=100000 path='foo/bar' -0.86 % ±0.91% ±1.20% ±1.55%
path/parse-win32.js n=100000 path='' 0.16 % ±1.16% ±1.53% ±1.97%
path/parse-win32.js n=100000 path='\\\\foo' *** -1.56 % ±0.84% ±1.11% ±1.42%
path/parse-win32.js n=100000 path='\\\\foo\\\\bar\\\\baz\\\\asdf\\\\.quux' * -1.82 % ±1.66% ±2.19% ±2.81%
path/parse-win32.js n=100000 path='C:\\\\' *** -2.60 % ±1.05% ±1.39% ±1.79%
path/parse-win32.js n=100000 path='C:\\\\foo' * -1.17 % ±1.08% ±1.43% ±1.84%
path/parse-win32.js n=100000 path='E:\\\\foo\\\\bar.baz' -0.56 % ±1.13% ±1.49% ±1.92%
path/parse-win32.js n=100000 path='foo\\\\.bar.baz' 0.27 % ±1.07% ±1.42% ±1.82%
path/parse-win32.js n=100000 path='foo\\\\bar' * -1.44 % ±1.24% ±1.64% ±2.11%
path/relative-posix.js n=100000 paths='/|/' *** -2.31 % ±1.21% ±1.59% ±2.05%
path/relative-posix.js n=100000 paths='/|/var' *** -4.80 % ±1.05% ±1.39% ±1.79%
path/relative-posix.js n=100000 paths='/data/orandea/test/aaa|/data/orandea/impl/bbb' *** -1.63 % ±0.32% ±0.43% ±0.55%
path/relative-posix.js n=100000 paths='/foo/bar/baz/quux|/' *** -1.50 % ±0.25% ±0.34% ±0.43%
path/relative-posix.js n=100000 paths='/foo/bar/baz/quux|/foo/bar/baz/quux' -0.42 % ±2.42% ±3.19% ±4.10%
path/relative-posix.js n=100000 paths='/foo/bar/baz/quux|/var/log' *** -1.23 % ±0.24% ±0.32% ±0.41%
path/relative-posix.js n=100000 paths='/var|/bin' *** -0.92 % ±0.50% ±0.66% ±0.85%
path/relative-win32.js n=100000 paths='C:\\\\|D:\\\\' 0.29 % ±0.87% ±1.15% ±1.48%
path/relative-win32.js n=100000 paths='C:\\\\foo\\\\bar\\\\baz\\\\quux|C:\\\\' 0.33 % ±0.56% ±0.73% ±0.94%
path/relative-win32.js n=100000 paths='C:\\\\foo\\\\bar\\\\baz|C:\\\\foo\\\\bar\\\\baz' 2.56 % ±2.64% ±3.48% ±4.47%
path/relative-win32.js n=100000 paths='C:\\\\foo\\\\BAR\\\\BAZ|C:\\\\foo\\\\bar\\\\baz' *** 1.07 % ±0.36% ±0.48% ±0.62%
path/relative-win32.js n=100000 paths='C:\\\\orandea\\\\test\\\\aaa|C:\\\\orandea\\\\impl\\\\bbb' -0.03 % ±0.27% ±0.36% ±0.46%
path/resolve-posix.js n=100000 paths='' 0.24 % ±0.58% ±0.77% ±0.99%
path/resolve-posix.js n=100000 paths='|' 0.08 % ±0.82% ±1.08% ±1.39%
path/resolve-posix.js n=100000 paths='a/b/c/|../../..' -0.03 % ±0.43% ±0.56% ±0.72%
path/resolve-posix.js n=100000 paths='foo/bar|/tmp/file/|..|a/../subfile' 0.43 % ±0.50% ±0.65% ±0.84%
path/resolve-win32.js n=100000 paths='' 0.56 % ±0.60% ±0.79% ±1.01%
path/resolve-win32.js n=100000 paths='|' -0.11 % ±0.76% ±1.01% ±1.30%
path/resolve-win32.js n=100000 paths='c:/blah\\\\blah|d:/games|c:../a' 0.00 % ±0.49% ±0.65% ±0.84%
path/resolve-win32.js n=100000 paths='c:/ignore|d:\\\\a/b\\\\c/d|\\\\e.exe' -0.27 % ±0.86% ±1.13% ±1.45%
path/toNamespacedPath-posix.js n=100000 path='.' *** -20.23 % ±0.97% ±1.28% ±1.65%
path/toNamespacedPath-posix.js n=100000 path='' *** -21.27 % ±1.36% ±1.79% ±2.30%
path/toNamespacedPath-posix.js n=100000 path='/home/node/..' *** -19.78 % ±0.80% ±1.06% ±1.36%
path/toNamespacedPath-posix.js n=100000 path='/tmp/bar' *** -19.69 % ±0.91% ±1.20% ±1.54%
path/toNamespacedPath-posix.js n=100000 path='/w/bnch-comp/node/benchmark' *** -19.40 % ±0.89% ±1.17% ±1.51%
path/toNamespacedPath-posix.js n=100000 path='bar/baz' *** -20.50 % ±1.00% ±1.32% ±1.70%
path/toNamespacedPath-win32.js n=100000 path='' *** -5.66 % ±1.83% ±2.41% ±3.10%
path/toNamespacedPath-win32.js n=100000 path='\\\\e.exe' 0.48 % ±1.46% ±1.93% ±2.48%
path/toNamespacedPath-win32.js n=100000 path='\\\\w\\\\bnch-comp\\\\node\\\\benchmark' 0.20 % ±0.70% ±0.92% ±1.19%
path/toNamespacedPath-win32.js n=100000 path='c:../a' * -0.38 % ±0.36% ±0.48% ±0.61%
path/toNamespacedPath-win32.js n=100000 path='c:/blah\\\\blah' ** -1.70 % ±1.17% ±1.54% ±1.98%
path/toNamespacedPath-win32.js n=100000 path='c:/ignore' -0.33 % ±1.46% ±1.93% ±2.48%
path/toNamespacedPath-win32.js n=100000 path='d:/games' * 1.40 % ±1.23% ±1.63% ±2.09%
path/toNamespacedPath-win32.js n=100000 path='d:\\\\a/b\\\\c/d' -0.78 % ±0.84% ±1.10% ±1.42%
Looks good overall, the only benchmark with significant performance impact is path/toNamespacedPath-posix.js.
IMHO it's good enough to land as is.
For context, originally path.posix.toNamespacedPath() is a noop. I doubt that this function is popular in userspace, but it's heavily used in Node.js internals (mostly in pair with getValidatedPath(), and in internal/modules).
There is an opened PR moving most of it to c++ (#52135) that should disconnect fs from performance impacts for js version of toNamespacedPath().
Alternatively on js side, I think we can reduce performance penalty in internals by introducing two versions of toNamespacedPath(): internal API without URL support and as public API with it.
@anonrig WDYT?
Member
Author
|
While the performance impact seems significant, I don't think it'll be noticeable (but I haven't tested it yet for the actual MS timings). It's just an extra As @LiviaMedeiros said, I could also make an internal function for it to be used in the internals. |
Member
|
Can we run the benchmarks of file system operations? I don't think 20% is a cost that is acceptable. |
Member
|
ResultsFWIW 20% was observed only for |
Member
Author
|
@anonrig, given the benchmark results, do you think a secondary system w/o URL support should be added for the |
Member
|
I'm -0.5 since benchmarks seem to have some impact, but I'd like to raise awareness before unblocking. cc @nodejs/tsc @nodejs/performance wdyt? |
GeoffreyBooth
left a comment
GeoffreyBooth
left a comment
Member
There was a problem hiding this comment.
Should we add an error hint where if someone calls a path method with a string URL (not an URL instance) we suggest that they wrap their input in new URL? Basically if we’re about to throw anyway, if the input started with file: then enrich the error with this suggestion.
Member
Author
IIRC It's possible for a file path to contain the |
Member
|
I suspect this will thrown when using permission model |
github-actions
Bot
removed
the
request-ci
Collaborator
Member
Author
I don't think that'll happen, as it's doing all the changes before any path processing really occurs |
anonrig
requested changes
May 4, 2024
anonrig
left a comment
anonrig
left a comment
Member
There was a problem hiding this comment.
The 2 referenced issues are 2 years old and 9 months old that didn't receive as much traffic to warrant for this change in an extremely important part of the Node.js core.
I prefer this function to not change the signature of the existing functions, if the request/need for the change is not warranting the cost of touching an extremely hot path. We might expose a different function that we don't rely on (internally), but I'm not sure.
I'm -1 on the proposed change as it is.
cc @nodejs/url
tniessen
reviewed
May 4, 2024
tniessen
left a comment
tniessen
left a comment
Member
There was a problem hiding this comment.
I am not convinced that this is a step in the right direction.
For example, isn't the second argument of the URL constructor supposed to effectively do resolve() (with the typical URL caveats)?
On a related note, normalize() has a variety of issues because the resulting path is not necessary equivalent to the original path. Explicitly propagating these quirks to file URLs feels odd to me.
MrJithil
reviewed
Aug 25, 2024
| validateString, | ||
| } = require('internal/validators'); | ||
| const { | ||
| toPathIfFileURL: URLToPathIfFileURL, |
Member
There was a problem hiding this comment.
Preferably, a variable with camelCasing. I can see its little complex as the existing functions already reserved the names.
However, something like getPathFromFileURL or something similar would be nice to have.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
12 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR changes the path functions to support File URLs.
Related to #41521
Related to #49273
Successor of #52497